Skip to content

Signal controlling constants for windows #1626

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Signal controlling constants for windows #1626

wants to merge 1 commit into from

Conversation

vorner
Copy link
Contributor

@vorner vorner commented Dec 16, 2019

SIG_DFL and similar.

Closes #1600.

@rust-highfive
Copy link

r? @gnzlbg

(rust_highfive has picked a reviewer for you, use r? to override)

@vorner
Signal controlling constants for windows
240f0e0 
SIG_DFL and similar.

Closes #1600.
gnzlbg
gnzlbg reviewed Dec 23, 2019
View reviewed changes
Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @retep998 could you review as well?

libc-test/build.rs
@@ -465,7 +465,9 @@ fn test_windows(target: &str) {
match name {
// FIXME: API error:
// SIG_ERR type is "void (*)(int)", not "int"
"SIG_ERR" => true,
"SIG_ERR" |
// Similar for SIG_DFL/IGN/GET/SGE/ACK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add them with the right type ?

retep998
retep998 reviewed Dec 23, 2019
View reviewed changes
src/windows/mod.rs
@@ -233,7 +233,13 @@ pub const SIGSEGV: ::c_int = 11;
pub const SIGTERM: ::c_int = 15;
pub const SIGABRT: ::c_int = 22;
pub const NSIG: ::c_int = 23;

pub const SIG_ERR: ::c_int = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#define SIG_ERR ((_crt_signal_t)-1)

Also libc defines sighandler_t as usize when in reality it is:
typedef void (__CRTDECL* _crt_signal_t)(int);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also libc defines sighandler_t as usize when in reality it is:
typedef void (__CRTDECL* _crt_signal_t)(int);

I'll take a PR to fix this, but I'll prefer if this change were in a different PR, and not here. We probably need to do a crater run while rolling in this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just keep in mind we can't naively use extern "C" fn(c_int) as the type because SIG_DFL has a value of 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need Option<extern "C" fn(c_int) -> ()> or similar (I'm not sure what to do about the __CRTDECL or whether extern "system" or some other ABI has to be used for windows.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__CRTDECL is __cdecl which is extern "cdecl" or extern "C". Most of the CRT uses __CRTDECL.

retep998
retep998 reviewed Dec 23, 2019
View reviewed changes
@vorner
Copy link
Contributor Author

vorner commented Dec 25, 2019

Hello

To be honest, I'm a bit lost here what the next steps from me should be. I can go and send a pull request, changing the type. However, there are few issues.

  • I don't have an easy access to windows machine, so I won't be able to do even basic testing.
  • The Option solves the problem of 0. Is it however OK to assign values like 3 to it? Doesn't it mandate the „reference“ to function is valid?

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 27, 2019

The Option solves the problem of 0. Is it however OK to assign values like 3 to it? Doesn't it mandate the „reference“ to function is valid?

cc @rust-lang/wg-unsafe-code-guidelines

@retep998
Copy link
Member

If only Rust had raw function pointers.

@RalfJung
Copy link
Member

Function pointers have to be non-null and aligned. While it seems unlikely that we'll ever have a platform where the alignment requirement is non-trivial, that's also not excluded currently. Cc rust-lang/unsafe-code-guidelines#197

@vorner
Copy link
Contributor Author

vorner commented Dec 31, 2019 •
edited
Loading

Reading the guide and considering this is for Windows platform only, which has quite limited support for HW platforms it runs on, I think this should be safe then. I'll prepare the other PR soon.

@retep998
Copy link
Member

Probably saner to use a raw pointer to an uninhabited type, since it has the same ABI as function pointers but we don't actually care about it being a callable function pointer.

@vorner
Copy link
Contributor Author

vorner commented Dec 31, 2019

But then it wouldn't really match the correct type… and then, is it worth changing at all, instead of staying as usize? I mean, there'll be some work and churn and crater runs… and if the result will be a different type that doesn't match the reality, then it seems like little benefit.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 2, 2020

How does this work in C?

IIRC, C requires function pointers to be properly aligned as well, at least when you call them, right ?

But then it wouldn't really match the correct type… and then, is it worth changing at all, instead of staying as usize?

I think we should move the discussion about how to "fix" the function pointer type to a different issue. This PR is not the place to do this (CI is broken right now, but once it works again, I'll merge this).

@vorner vorner mentioned this pull request Jan 5, 2020
Open
@bors
Copy link
Contributor

bors commented May 25, 2021

☔ The latest upstream changes (presumably #2194) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link

This is ready to merge, right, apart from rebase?

@vorner
Copy link
Contributor Author

vorner commented May 27, 2021

I think so…

CI is broken right now, but once it works again, I'll merge this

Except I must have deleted my fork of the repo in the meantime during some cleanup :-|. I can re-create it, but I'm afraid that'll mean a new PR :-( ‒ I don't expect github UI to handle it reappearing well.

@vorner vorner mentioned this pull request May 27, 2021
Merged
@vorner
Copy link
Contributor Author

vorner commented May 27, 2021

Indeed, it's as I expected, github didn't pick it up even if the branch has the same name. It's in #2201 now, with the same content.

And apologies for the confusion :-|

@vorner vorner closed this May 27, 2021
bors added a commit that referenced this pull request Jun 1, 2021
@bors
Auto merge of #2201 - vorner:winsig, r=JohnTitor
6b0224e 
Signal controlling constants for windows

This is replacement of #1626, because I have lost the repository with that branch in the meantime. The content is the same (unless I've made some mistake copying it over), just rebased onto current master.
bors added a commit that referenced this pull request Jun 1, 2021
@bors
Auto merge of #2201 - vorner:winsig, r=JohnTitor
4952853 
Signal controlling constants for windows

This is replacement of #1626, because I have lost the repository with that branch in the meantime. The content is the same (unless I've made some mistake copying it over), just rebased onto current master.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@retep998 retep998 retep998 left review comments

@gnzlbg gnzlbg gnzlbg left review comments

Assignees

@gnzlbg gnzlbg

Labels
S-waiting-on-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SIG_DFL and SIG_IGN on windows
8 participants
@vorner @rust-highfive @gnzlbg @retep998 @RalfJung @bors @nikomatsakis @JohnTitor